Fix Netty 4.1 HTTP/1.1 pipelined response tracing#11937
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 57f9d65 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
6f4df04 to
7f7fdf4
Compare
There was a problem hiding this comment.
More details
The PR correctly fixes HTTP/1.1 pipelining by switching from single-attribute storage to a per-channel queue, allowing multiple in-flight requests to maintain independent tracing contexts. Code review of request queuing, response FIFO matching, error handling, and channel cleanup pathways reveals no behavioral regressions—the fix is architecturally sound.
🤖 Datadog Autotest · Commit 7f7fdf4 · What is Autotest? · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
Pull request overview
Fixes Netty 4.1 HTTP/1.1 pipelined response tracing by tracking per-request server state in a per-channel FIFO queue, so each outbound response is matched to the correct inbound request span/context (including AppSec response blocking state and request header context).
Changes:
- Introduces
ServerRequestContextto queue per-request tracing + AppSec state on the channel and safely disable tracing when backlog grows beyond a limit. - Updates Netty 4.1 server handlers to consume/advance this queue when writing responses and when blocking responses.
- Adds regression tests for HTTP/1.1 pipelining span creation and for the pending-context limit behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/ServerRequestContext.java | New per-channel FIFO queue of per-request server tracing/AppSec state for pipelined matching. |
| dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/AttributeKeys.java | Removes now-obsolete per-channel request/response state keys; exposes attributeKey for queue key creation. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java | Switches AppSec response analysis/blocking bookkeeping to per-request queued context. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerResponseTracingHandler.java | Finishes/removes the correct request span/context for each response via queued context. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerRequestTracingHandler.java | Enqueues per-request context and drains all pending contexts on channel close. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelPipelineInstrumentation.java | Ensures ServerRequestContext is injected as a helper alongside Netty 4.1 server/client helpers. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelHandlerContextInstrumentation.java | Adds helper injection for ServerRequestContext and minor refactor in fire* activation. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/ChannelFutureListenerInstrumentation.java | Adds helper injection for ServerRequestContext in the module helper set. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/ServerRequestContextTest.java | Unit test validating tracing is disabled and contexts are drained when the pending limit is exceeded. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/NettyHttp11PipeliningTest.java | Regression test asserting spans are created correctly for pipelined HTTP/1.1 requests/responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** Per-request server state stored on the channel until the matching response is written. */ | ||
| public final class ServerRequestContext { | ||
| /** Returns whether a new server request can be tracked on this channel. */ |
| final ServerRequestContext currentContext = contexts.peekLast(); | ||
| if (currentContext == null) { | ||
| attributes.attr(CONTEXT_ATTRIBUTE_KEY).remove(); | ||
| } else { | ||
| // Keep the legacy mirror pointed at the current inbound request after removing an older | ||
| // response context. | ||
| attributes.attr(CONTEXT_ATTRIBUTE_KEY).set(currentContext.tracingContext()); | ||
| } |
| private final Context tracingContext; | ||
| private final HttpHeaders requestHeaders; | ||
| private boolean responseAnalyzed; | ||
| private boolean responseBlocked; | ||
|
|
||
| public Context tracingContext() { | ||
| return tracingContext; | ||
| } | ||
|
|
||
| public HttpHeaders requestHeaders() { | ||
| return requestHeaders; | ||
| } |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57f9d65709
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (serverContext.isResponseAnalyzed()) { | ||
| if (serverContext.isResponseBlocked()) { |
There was a problem hiding this comment.
Keep dropping writes after a response block
When AppSec blocks a streaming response, this state now lives only on the head ServerRequestContext; the replacement blocking FullHttpResponse is written through HttpServerResponseTracingHandler, which immediately removes that context while the channel close is still asynchronous. Any subsequent original response chunks written before the close then see no blocked context (or the next pipelined request's context) and are forwarded instead of being released, regressing the previous channel-level blocked flag behavior for non-FullHttpResponse responses.
Useful? React with 👍 / 👎.
What Does This Do
Fixes Netty 4.1 HTTP/1.1 pipelining by storing server request state in a per-channel queue instead of a single overwritten channel attribute.
Each queued entry now carries the tracing context, request headers, and AppSec response blocking state, so outbound responses are matched with the correct inbound request.
Motivation
HTTP/1.1 pipelining can place multiple in-flight requests on the same Netty channel. The previous instrumentation kept only one server tracing context per connection, so later requests overwrote earlier request state before their responses were written. This caused responses to finish or annotate the wrong span, losing trace context for pipelined requests.
Additional Notes
CONTEXT_ATTRIBUTE_KEYis still mirrored for generic Nettyfire*span activation.channelInactive.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]